Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/524/weights in beta #532

Merged
merged 45 commits into from
Mar 28, 2024
Merged

Issue/524/weights in beta #532

merged 45 commits into from
Mar 28, 2024

Conversation

eduardojsbarroso
Copy link
Collaborator

Include weights in beta measurements #524

@eduardojsbarroso eduardojsbarroso self-assigned this Jul 21, 2022
@eduardojsbarroso eduardojsbarroso linked an issue Jul 21, 2022 that may be closed by this pull request
@eduardojsbarroso
Copy link
Collaborator Author

@marina-ricci Is this what you were thinking about? Let me know if you want something different.

@coveralls
Copy link

coveralls commented Jul 21, 2022

Coverage Status

coverage: 100.0%. remained the same
when pulling b044dd7 on issue/524/weights_in_beta
into afa77f0 on main.

@m-aguena
Copy link
Collaborator

[from 25/11/2022 tag-up]: wait for PR #517

@m-aguena m-aguena added HasDependency Depends on another issue to be done and removed HasDependency Depends on another issue to be done labels Jan 23, 2023
@eduardojsbarroso
Copy link
Collaborator Author

@m-aguena I think everything is working now. I added the option of the user to provide shape weights for the source galaxies. If you can take a look if this is what you want please.

@eduardojsbarroso eduardojsbarroso marked this pull request as ready for review March 2, 2023 14:08
Copy link
Collaborator

@m-aguena m-aguena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall is ok. I made comments in specific lines, but they apply to several modifications.

clmm/utils.py Outdated
@@ -782,8 +782,7 @@ def integrand(z_i, z_cl=z_cl, cosmo=cosmo):
B_mean = quad(integrand, zmin, zmax)[0] / quad(z_distrib_func, zmin, zmax)[0]
return B_mean

def compute_beta_s_mean(z_cl, z_inf, cosmo, zmax=10.0, delta_z_cut=0.1, zmin=None,
z_distrib_func=None):
def compute_beta_s_mean(z_cl, z_inf, cosmo, zmax=10.0, delta_z_cut=0.1, zmin=None, z_distrib_func=None, weights_option=False, z_src = None, shape_weights = None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the weights_option argument and just apply weight when shape_weights is provided

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

clmm/utils.py Outdated
zmin = z_cl + delta_z_cut
Bs_mean = quad(integrand, zmin, zmax)[0] / quad(z_distrib_func, zmin, zmax)[0]
if weights_option == False:
if z_distrib_func == None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use is None instead of == None

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

clmm/utils.py Outdated
else:
n_galaxies = len(z_src)
weight = 1/n_galaxies
Bsw_i = [shape_weights[i] * compute_beta_s(z_src[i], z_cl, z_inf, cosmo) for i in range(0,len(z_src))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe a simple vector product can be used here shape_weights * compute_beta_s(z_src, z_cl, z_inf, cosmo)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

To change that, I had to change compute_beta to accept a list of sources instead of being called to compute each source at a time. I changed the documentation for z_source: float, array_like, but if the user passes a scalar just for one source, I'm transforming into a one argument list.

clmm/utils.py Outdated
beta = np.heaviside(z_src-z_cl, 0) * cosmo.eval_da_z1z2(z_cl, z_src) / cosmo.eval_da(z_src)
return beta
if type(z_src) is not list:
z_src = [z_src]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I should keep this test or expect the user to pass a list with just one source. @m-aguena

@m-aguena
Copy link
Collaborator

@combet @marina-ricci @eduardojsbarroso I was checking this use, and to sum up we have two possible ways of computing <beta_s>, <beta_s^2>:

  • From N(z):
    image
  • From weights:
    image

My suggestion here is to just have different functions (ex: beta_from_distribution, beta_from_weights) as the inputs for each one are completely different.

Then we have two options for how this will be used in the theory functions (ex: eval_tangential_shear):

  1. The user has to pre-compute <beta_s>, <beta_s^2>, and pass it to theory using z_src=(<beta_s>, <beta_s^2>), z_src_model='beta'
  2. We add the possibility for this computation to be done inside the modeling functions. In this case we have to add an option like z_src_info='beta_from_weights' with z_src=(src_redshifts, src_weights)

Opinions?

@eduardojsbarroso
Copy link
Collaborator Author

@combet @marina-ricci @eduardojsbarroso I was checking this use, and to sum up we have two possible ways of computing <beta_s>, <beta_s^2>:

  • From N(z):
    image
  • From weights:
    image

My suggestion here is to just have different functions (ex: beta_from_distribution, beta_from_weights) as the inputs for each one are completely different.

Then we have two options for how this will be used in the theory functions (ex: eval_tangential_shear):

  1. The user has to pre-compute <beta_s>, <beta_s^2>, and pass it to theory using z_src=(<beta_s>, <beta_s^2>), z_src_model='beta'
  2. We add the possibility for this computation to be done inside the modeling functions. In this case we have to add an option like z_src_info='beta_from_weights' with z_src=(src_redshifts, src_weights)

Opinions?

I think the second option is better than to having the users to compute themselves.

@marina-ricci
Copy link
Collaborator

@combet @marina-ricci @eduardojsbarroso I was checking this use, and to sum up we have two possible ways of computing <beta_s>, <beta_s^2>:

  • From N(z):
    image
  • From weights:
    image

My suggestion here is to just have different functions (ex: beta_from_distribution, beta_from_weights) as the inputs for each one are completely different.
Then we have two options for how this will be used in the theory functions (ex: eval_tangential_shear):

  1. The user has to pre-compute <beta_s>, <beta_s^2>, and pass it to theory using z_src=(<beta_s>, <beta_s^2>), z_src_model='beta'
  2. We add the possibility for this computation to be done inside the modeling functions. In this case we have to add an option like z_src_info='beta_from_weights' with z_src=(src_redshifts, src_weights)

Opinions?

I think the second option is better than to having the users to compute themselves.

I actually think we can live with option 1 (for which we do not have to make modifications) for the time being. I think it would be cleaner that way.

@eduardojsbarroso
Copy link
Collaborator Author

@m-aguena tell me if this was what you were thinking when you have time, please.

@eduardojsbarroso
Copy link
Collaborator Author

@m-aguena Do you know why is the test failling? The tets work in my computer.

@m-aguena
Copy link
Collaborator

m-aguena commented May 9, 2023

@eduardojsbarroso yes, it has to do with CCL's new version. PR #593 should fix it

@m-aguena
Copy link
Collaborator

@m-aguena There are a lot of functions in parent_class that don't have the option distribution anymore, while in the notebook they were computed previously. This option should be added to all the functions or should I remove from the notebook?

@eduardojsbarroso They should be removed from the notebooks. Now, beta should be computed by the user and passed to these function, and this should be reflected on the notebooks

@m-aguena
Copy link
Collaborator

m-aguena commented Jul 13, 2023

@combet @marina-ricci can one of you guys take a final look before I merge this? I would like a second pair of eyes on it since there are some API changes.

The main implementation was the suggestion here #532 (comment)

But I also ended up adding the functions _eval_da, _eval_da_z1z2, _get_z_from_a, _get_a_from_z to the cosmology object for completeness and using them internally.

Thanks!

@m-aguena m-aguena merged commit 50876a8 into main Mar 28, 2024
2 checks passed
combet pushed a commit that referenced this pull request May 7, 2024
* distinguish between compute_beta_s_*_from_distribution and compute_beta_s_*_from_weights

* make compute_beta more efficient and fix shape_weights=None in compute_beta_s_mean*_from_weights

* add _eval_da, _eval_da_z1z2, _get_z_from_a, _get_a_from_z to cosm

* reorder funcs in cosmo parent

* add fix to skip pylint for astropy.units (fails in astropy 6)

* Update version to 1.12.0

---------

Co-authored-by: m-aguena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include weights in beta measurements
4 participants